Skip to content

tests: prevent leaked tarantool and panic on bad start#586

Merged
oleg-jukovec merged 1 commit intomasterfrom
bigbes/gh-147-fix-invalid-startup-panics
May 5, 2026
Merged

tests: prevent leaked tarantool and panic on bad start#586
oleg-jukovec merged 1 commit intomasterfrom
bigbes/gh-147-fix-invalid-startup-panics

Conversation

@bigbes
Copy link
Copy Markdown
Collaborator

@bigbes bigbes commented May 5, 2026

Two related issues from #147 made test failures painful to diagnose:

  1. When a test panicked, the spawned tarantool process was left running and kept its TCP port bound, so the next run failed with address already in use instead of the original test failure.
  2. The common defer test_helpers.StopTarantoolWithCleanup(inst) placed before the require.NoError(t, err) panicked with a nil-pointer dereference when StartTarantool returned a nil instance on error, hiding the real "Unable to start Tarantool" failure.

Changes were made:

  • New test_helpers.commandKillOnExit helper: sets Pdeathsig=SIGTERM on Linux so the tarantool child dies with the test process. Darwin gets a no-op fallback (no Pdeathsig equivalent — see cmd_other.go).
  • Reordered every offending test site to assert StartTarantool succeeded before deferring cleanup. Standard Go idiom, and avoids growing a nil-guard inside the helper that would mask further StartTarantool bugs.

Part of #147.

@bigbes bigbes force-pushed the bigbes/gh-147-fix-invalid-startup-panics branch from 6dba26a to 0b5c01f Compare May 5, 2026 09:07
Copy link
Copy Markdown
Collaborator

@oleg-jukovec oleg-jukovec left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Extra dot in the commit message:

Part of #147. -> Part of #147

Two related issues from #147 made test failures painful to diagnose:

1. When a test panicked, the spawned tarantool process was left
   running and kept its TCP port bound, so the next run failed with
   "address already in use" instead of the original test failure.

2. The defer/require ordering used in many tests was

       inst, err := test_helpers.StartTarantool(opts)
       defer test_helpers.StopTarantoolWithCleanup(inst)
       require.NoError(t, err)

   When StartTarantool returned a nil instance on error (the connect
   failure path returns nil after stopping the half-started process),
   the deferred cleanup ran on a nil pointer and panicked, hiding the
   real "Unable to start Tarantool" failure under a nil-deref stack.

Fix the first by setting Pdeathsig=SIGTERM on the tarantool command
on Linux via a new test_helpers.commandKillOnExit helper, with a
no-op fallback for other platforms (darwin has no Pdeathsig
equivalent — see the comment in cmd_other.go).

Fix the second by reordering every offending test site to assert
StartTarantool succeeded before deferring the cleanup. This is the
standard Go idiom and avoids growing a nil-guard inside the helper
that would mask further StartTarantool bugs.

Part of #147
@oleg-jukovec oleg-jukovec force-pushed the bigbes/gh-147-fix-invalid-startup-panics branch from 0b5c01f to fe5c21d Compare May 5, 2026 21:46
@oleg-jukovec
Copy link
Copy Markdown
Collaborator

Extra dot in the commit message:

Part of #147. -> Part of #147

Fixed.

@oleg-jukovec oleg-jukovec merged commit 64dacc6 into master May 5, 2026
27 checks passed
@oleg-jukovec oleg-jukovec deleted the bigbes/gh-147-fix-invalid-startup-panics branch May 5, 2026 22:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants